✅ Deploy Preview for salsa-rs ready!

Name	Link
🔨 Latest commit	9570e80
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/680377b8f0253a00084567e9
😎 Deploy Preview	https://deploy-preview-798--salsa-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

CodSpeed Performance Report

Merging #798 will degrade performances by 4.9%

_{Comparing Veykril:veykril/push-wqwvkrpytswm (9570e80) with master (ab7ecb4)}

Summary

❌ 1 regressions
✅ 11 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Perhaps it is better to have a different name than cycle_initial, that implies something after the initial?

Should we add documentation for this in the book?

I think one thing that should be documented there is that using cycle_result is likely to cause non-deterministic results for a given query graph depending on the entry point.

Avoiding this is the reason the fallback feature of fixpoint iteration requires that a single extra iteration must converge.

I think one thing that should be documented there is that using cycle_result is likely to cause non-deterministic results for a given query graph depending on the entry point.

@Veykril can this cause problems for us? Definitely not for things that emit an error for cycles (e.g. layouts), since we'll just propagate the error, but what about stuff like generic_defaults()?

Barring one error message, this looks reasonable to me, as long as we are OK with the potential non-determinism.

(To be clear, non-determinism is also possible with fixpoint iteration, if you define the semantics of your queries badly. But the convergence requirement means you have to work significantly harder to get non-determinism, whereas with cycle_result non-determinism is likely to be the default outcome.)

The non-determinism comes from the lack of convergence by design, right?

The non-determinism comes from the lack of convergence by design, right?

I guess? Not entirely sure what you mean. But yes, non-determinism is an inevitable result of allowing the use of non-converging results in a cycle. So if you intend all of that, then it is all by design :)

Hm, is this really non-determinism? I mean from what I gather here, the result may potentially depend on the entry point, that is the order of execution. Ah, I guess that becomes non-deterministic when multiple threads are involved in the cycle?

Hmm, actually I think there is no non-determinism here (unlike with the immediate one-iteration fixpoint). I believe the order we enter a mutli-thread cycle does not matter, or rather, we can't enter a multi-thread cycle with this scheme as the cycle immediately resolves?

Though I haven't thought about what happens if we enter a fixpoint cycle that itself runs into a fallback cycle. That one might be problematic.

Either way I've added a parallel test based on cycle_a_t1_b_t2.rs for the fallback mechanism.

Also of note is that this scheme still differs from what salsa used to:

• Before, salsa replaced the result of the query that contained a cycle trigger with the fallback
• With this, salsa just returns the result of the cycle invocation, so the query that contains the cycle trigger continues to run with this provisional value instead.

@carljm Can you double check whether my comments are correct with your understanding of the fixpoint logic?

I believe this to not introduce non-determinism (if my understanding of things is right), in which case I'd merge this without the need to document non-determinism.

Yes, the difference you describe between this PR and previous Salsa fallback behavior is because previous Salsa was going to some lengths to avoid non-determinism, by replacing the result of every query in the cycle with a fallback value.

I don't think the non-determinism here has any dependency on multi-threading, it's simply dependent on entry-point into the cycle, which can vary even given a single thread. Consider two queries a and b where a returns 10 * b() and b returns 1 + a(), and they both have a cycle_result of 0. If you happen to execute a first, you will memoize final results of 10 for a() and 1 for b. If you happen to execute b first, you will memoize final results of 0 for a and 1 for b. This is non-determinism. The value memoized for a is not deterministic based on the definitions of the queries and the values of inputs; it depends also on the order in which queries were executed.

In terms of how this occurs in real usage scenarios, I don't know as much about r-a, but for red-knot, much of our analysis is lazy, so if we have a third-party module we aren't checking directly, we will query just the particular definitions we need from it. So with this kind of non-determinism, the types we infer for things in that module could change, even if that module and its dependencies don't change, depending on what we happen to ask for from it first (which could depend on some details of the first-party module we are checking.)

I guess this behavior is still deterministic if you consider the entire execution context as part of the "inputs", but it is non-deterministic in the sense that I think Salsa has usually used the term, in which the results of any given query subgraph should depend only on those queries and their inputs.

Perhaps there are salsa users for whom this behavior is fine. I don't object to offering it as an option, just think it should be clearly documented, because in the past Salsa has gone to some lengths to avoid it.

@Veykril Can't we do the same as old Salsa? That is, replacing the value of the cycle head with the fallback, and discarding every other query?

Right, I do consider the order of execution to make this deterministic (in a single threaded scenario), but you raised a good point, the order of queries that may depend on cycle set queries can give surprising results in some setups. I wonder if there is a some middle ground that can be struck here that would fulfill rust-analyzer's needs without incurring this problem.

Either way I agree in that case we should document this.

Hmm, now I do wonder though. This behavior specifically requires 2 or more cycle allowing queries. If the cycle only consists of one query type the result will still be deterministic, given there is only one such entry point. I believe that one alone is all rust-analyzer needs so I wonder if we can diagnose the other cases.

Hmm, okay looking at the rust-analyzer failures from before that limitations won't work for us it seems ... Maybe we should remodel our relevant queries here to become fallible for cycles, so that we can converge to an error instead.